Skip to content

Fix self-push CI report path in cat#17111

Merged
stas00 merged 2 commits intohuggingface:mainfrom
ydshieh:fix_push_ci_report_path
May 6, 2022
Merged

Fix self-push CI report path in cat#17111
stas00 merged 2 commits intohuggingface:mainfrom
ydshieh:fix_push_ci_report_path

Conversation

@ydshieh
Copy link
Collaborator

@ydshieh ydshieh commented May 6, 2022

What does this PR do?

self-push.yml has lines like

run: cat reports/tests_torch_gpu_failures_short.txt

which should be

run: cat reports/tests_torch_gpu/failures_short.txt

Currently, we get errors like

cat: reports/tests_torch_multi_gpu_failures_short.txt: No such file or directory
Error: Process completed with exit code 1.

(see for example this job).

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented May 6, 2022 •
edited
Loading

The documentation is not available anymore as the PR was closed or merged.

sgugger
sgugger reviewed May 6, 2022
View reviewed changes
Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ccing @stas00 since he introduced this.

If this change is needed, it should also be done in the other GitHub action files, and also in the report part of the job? Not sure if they will push properly nested files in subdirectories.

@ydshieh
Copy link
Collaborator Author

ydshieh commented May 6, 2022 •
edited
Loading

If this change is needed, it should also be done in the other GitHub action files,

The other occurrences are in self-nightly-scheduled.yml". I can update it too. Done

and also in the report part of the job? Not sure if they will push properly nested files in subdirectories.

I guess you are talking about files like notification_service_deprecated.py and notification_service.py. They are not using failures_short.txt.

Note that:

  • this PR doesn't change any artifact directory location
  • notification_service_deprecated.py does have issues with the report.
    • For example, "run_all_tests_tf_gpu_test_reports/[X].txt" should be run_all_tests_tf_gpu_test_reports/tests_torch_gpu/[X].txt
      • (if we don't change the artifact directory in self-push.yml)
    • I am still on the task of the self-push report format, which might still take some time.
      • But think I can already fix a few small things in the meantime.

stas00
stas00 approved these changes May 6, 2022
View reviewed changes
Copy link
Contributor

@stas00 stas00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for fixing, @ydshieh

Clearly nobody was looking at the reports ;)

The original was correct, but when this new workflow was added it had this bug

@stas00 stas00 merged commit 351cdbd into huggingface:main May 6, 2022
@ydshieh ydshieh deleted the fix_push_ci_report_path branch May 6, 2022 14:53
elusenji pushed a commit to elusenji/transformers that referenced this pull request Jun 12, 2022
@ydshieh @elusenji
Fix self-push CI report path in cat (huggingface#17111)
259cda8 
* fix report cat path

* fix report cat path

Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@patrickvonplaten patrickvonplaten Awaiting requested review from patrickvonplaten

2 more reviewers

@sgugger sgugger sgugger left review comments

@stas00 stas00 stas00 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ydshieh @HuggingFaceDocBuilderDev @stas00 @sgugger